content: Support the new class of channel wildcard mentions #1073

sm-sayedi · 2024-11-22T06:14:04Z

For channel wildcard mentions, the class in the corresponding HTML used to be the same as the user mentions (class="user-mention"), but now there's an additional class added. Now it looks like the following: class="user-mention channel-wildcard-mention".

Fixes: #1064

PIG208

Thanks! This looks good. Left some comments.

PIG208 · 2024-11-25T22:16:20Z

lib/model/content.dart

+    const mentionClass = r"user(?:-group)?-mention|"
+                          "(?:user-mention channel-wildcard-mention)";
+    return RegExp("^(?:$mentionClass)(?: silent)?|silent (?:$mentionClass)\$");


Looks like the non-capturing groups added here are not necessary:

Suggested change

const mentionClass = r"user(?:-group)?-mention|"

"(?:user-mention channel-wildcard-mention)";

return RegExp("^(?:$mentionClass)(?: silent)?|silent (?:$mentionClass)\$");

const mentionClass = r"user(?:-group)?-mention|"

"user-mention channel-wildcard-mention";

return RegExp("^(?:$mentionClass)(?: silent)?|silent $mentionClass\$");

Seeing this I wondered if regular expressions are the right tool for this issue, but then I read from the commit message of 4a10b3c mentioning that this is more efficient than classes. Makes sense.

The groups around mentionClass are needed. The revised version above would have a three-way alternative: either ^(?:$mentionClass)(?: silent)?, or silent user(?:-group)?-mention, or user-mention channel-wildcard-mention$.

Probably the cleanest way to express that is by mentionClass itself containing the (?: and ).

In main there's also a group enclosing everything between ^ and $. That's necessary so that ^ and $ apply to all alternatives, instead of just the first and last of them respectively.

I've just sent #1086 to refactor the existing logic here, reducing the role of the regexp and hopefully making it easier to add more features. @sm-sayedi PTAL and try rebasing your work atop that.

sm-sayedi · 2024-11-27T05:12:34Z

Thanks @PIG208 and @gnprice for the previous comments. Pushed the new changes with the branch being rebased on top of #1086. PTAL.

PIG208 · 2024-12-03T20:26:34Z

lib/model/content.dart

+    bool isChannelWildcardClassIncluded = false;
+    if (classes[i] == 'channel-wildcard-mention') {
+      // A newly-added class for channel wildcard mentions.
+      // See: https://github.com/zulip/zulip/pull/31075


The example is contained in our tests, so we can probably cut this comment. And the context of "newly-added" might fit better in the commit message.

(On what goes in a commit message vs. a comment, see: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#commit-messages-vs-comments )

gnprice

Thanks @sm-sayedi for taking care of this, and @PIG208 for the previous reviews! This all looks good; some nits below.

gnprice · 2024-12-03T22:27:06Z

lib/model/content.dart

+    int i = 0;
+
+    if (i >= classes.length) return null;
+    bool isChannelWildcardClassIncluded = false;


nit: can simplify "is X included" to "has X":

Suggested change

bool isChannelWildcardClassIncluded = false;

bool hasChannelWildcardClass = false;

gnprice · 2024-12-03T22:33:10Z

lib/model/content.dart

+    if (classes[i] == 'channel-wildcard-mention') {
+      // A newly-added class for channel wildcard mentions.
+      // See: https://github.com/zulip/zulip/pull/31075


I think a comment is useful here, because there is certain information it can convey that's relevant for the reader trying to understand this function. Here's a version focused on that information:

Suggested change

if (classes[i] == 'channel-wildcard-mention') {

// A newly-added class for channel wildcard mentions.

// See: https://github.com/zulip/zulip/pull/31075

if (classes[i] == 'channel-wildcard-mention') {

// Newer channel wildcard mentions have this class; older ones don't.

In particular, in my other review comment above I almost suggested naming the variable isChannelWildcard — before I remembered that that would be an inaccurate name, because some genuine channel wildcards don't have this class. That fact will be highly relevant when someone goes to implement #646, as it'd otherwise be tempting to use this class for doing so.

gnprice · 2024-12-03T22:37:59Z

lib/model/content.dart

+    if (classes[i] == 'user-mention' ||
+       (classes[i] == 'user-group-mention' && !isChannelWildcardClassIncluded)) {


nit: indentation; and operators go after newline:

Suggested change

if (classes[i] == 'user-mention' ||

(classes[i] == 'user-group-mention' && !isChannelWildcardClassIncluded)) {

if (classes[i] == 'user-mention'

|| (classes[i] == 'user-group-mention' && !isChannelWildcardClassIncluded)) {

(See examples in parseInlineContent below.)

With less indentation, the second sub-condition looks like it's a peer of the if condition as a whole.

For the rationale for || after newline, see:
https://github.com/zulip/zulip-mobile/blob/a115df1f71c9dc31e9b41060a8d57b51c017d786/tools/formatting.eslintrc.yaml#L20-L23

gnprice · 2024-12-03T22:39:59Z

test/model/content_test.dart

@@ -1213,7 +1255,13 @@ void main() {
    testParseExample(ContentExample.groupMentionSilent);
    testParseExample(ContentExample.groupMentionSilentClassOrderReversed);

-    // TODO test wildcard mentions


Cool, good to have this handled 🙂

gnprice · 2024-12-03T22:43:26Z

test/model/content_test.dart

+    '<p><span class="silent user-mention channel-wildcard-mention" data-user-id="*">channel</span></p>',
+    const UserMentionNode(nodes: [TextNode('channel')]));
+
+  static final legacyChannelWildcardMentionPlain = ContentExample.inline(


Yeah, this naming is good: the current/new way gets the simple name, and the old way gets the longer "legacy" name.

sm-sayedi · 2024-12-04T05:43:02Z

Thanks @gnprice and @PIG208 for the reviews! Changes pushed.

~~Edit: CI failed for some reason; I will look at it and fix it!~~

New edit: CI failure is fixed now!

gnprice

Thanks for the revision! Looks good, with one nit below. I'll fix that and merge.

gnprice · 2024-12-05T04:05:36Z

lib/model/content.dart

-      // A silent @-mention.  We ignore this flag; see [UserMentionNode].
+      // A silent @-mention. We ignore this flag; see [UserMentionNode].


nit: extraneous change

For channel wildcard mentions, the class in the corresponding HTML used to be the same as the user mentions (class="user-mention"), but now there's an additional class added. Now it looks like the following: class="user-mention channel-wildcard-mention". Fixes: zulip#1064

sm-sayedi added the maintainer review PR ready for review by Zulip maintainers label Nov 22, 2024

gnprice requested a review from PIG208 November 25, 2024 21:45

PIG208 self-assigned this Nov 25, 2024

PIG208 reviewed Nov 25, 2024

View reviewed changes

gnprice mentioned this pull request Nov 25, 2024

content [nfc]: Pull out parseUserMention, give regexp less to do #1086

Merged

sm-sayedi force-pushed the 1064-channel-wildcard-mentions branch 2 times, most recently from 20c3911 to 79b117d Compare November 27, 2024 05:09

PIG208 reviewed Dec 3, 2024

View reviewed changes

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Dec 3, 2024

PIG208 assigned gnprice Dec 3, 2024

PIG208 requested a review from gnprice December 3, 2024 20:27

gnprice reviewed Dec 3, 2024

View reviewed changes

sm-sayedi force-pushed the 1064-channel-wildcard-mentions branch from 79b117d to 9c3d023 Compare December 4, 2024 05:41

sm-sayedi force-pushed the 1064-channel-wildcard-mentions branch 2 times, most recently from 29e0154 to 698a93b Compare December 5, 2024 02:29

gnprice reviewed Dec 5, 2024

View reviewed changes

gnprice force-pushed the 1064-channel-wildcard-mentions branch from 698a93b to fb6291f Compare December 5, 2024 04:09

gnprice merged commit fb6291f into zulip:main Dec 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

content: Support the new class of channel wildcard mentions #1073

content: Support the new class of channel wildcard mentions #1073

sm-sayedi commented Nov 22, 2024

PIG208 left a comment

PIG208 Nov 25, 2024

PIG208 Nov 25, 2024

gnprice Nov 25, 2024

gnprice Nov 25, 2024

sm-sayedi commented Nov 27, 2024

PIG208 Dec 3, 2024

gnprice Dec 3, 2024

gnprice left a comment

gnprice Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

gnprice Dec 3, 2024

sm-sayedi commented Dec 4, 2024 •

edited

Loading

gnprice left a comment

gnprice Dec 5, 2024

	bool isChannelWildcardClassIncluded = false;
	bool hasChannelWildcardClass = false;

		if (classes[i] == 'user-mention' \|\|
		(classes[i] == 'user-group-mention' && !isChannelWildcardClassIncluded)) {

		// A silent @-mention. We ignore this flag; see [UserMentionNode].
		// A silent @-mention. We ignore this flag; see [UserMentionNode].

content: Support the new class of channel wildcard mentions #1073

content: Support the new class of channel wildcard mentions #1073

Conversation

sm-sayedi commented Nov 22, 2024

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-sayedi commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-sayedi commented Dec 4, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-sayedi commented Dec 4, 2024 •

edited

Loading